Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[24.0] Update title of edited PRs only if base ref changed #19183

Conversation

nsoranzo
Copy link
Member

Also:

  • Update title of reopened PRs too.
  • Don't unnecessarily checkout the repo.

Alternative to #19177 .

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@nsoranzo nsoranzo changed the base branch from dev to release_24.0 November 21, 2024 17:55
@github-actions github-actions bot added this to the 25.0 milestone Nov 21, 2024
@github-actions github-actions bot changed the title Update title of edited PRs only if base ref changed [24.0] Update title of edited PRs only if base ref changed Nov 21, 2024
@github-actions github-actions bot modified the milestones: 25.0, 24.1 Nov 21, 2024
@jdavcs
Copy link
Member

jdavcs commented Nov 21, 2024

So does this fire when the pull request was opened or reopened, or edited AND had its base branch changed?

@nsoranzo
Copy link
Member Author

So does this fire when the pull request was opened or reopened, or edited AND had its base branch changed?

Yes, if you just change the title or description of the PR, the CI job is skipped.

@jdavcs
Copy link
Member

jdavcs commented Nov 21, 2024

So does this fire when the pull request was opened or reopened, or edited AND had its base branch changed?

Yes, if you just change the title or description of the PR, the CI job is skipped.

But then this doesn't really address the concerns discussed in #19177. Yes, we can fix the title by editing just the title as a separate follow-up edit, after it has been renamed incorrectly by the bot - but only if we remember to do so - which we might not (someone might not even know they should do that). I'm sorry, I still think we should not enable automated renaming on edit until the bot handles all those cases correctly.

@nsoranzo
Copy link
Member Author

It only works one way. So when you re-target from dev to release_24.2, you'll get the renaming, but when you re-target the opposite way (which is not an impossible scenario), it won't remove the prefix. If you have to add it manually in the first case, you'll remember to remove it in the second, otherwise you're likely to forget.

This would obviously be nice to have, but was never part of the initial implementation, so it can't be held against this bugfix.

What if we want to re-target from one release to another? You'll end up with two prefixes in the title.

This PR is an incremental improvement, we don't need to fix all possible scenarios at once since it's nothing mission-critical. Which was also the reasoning when we merged #18835 .

If I manually edit the title, that implies that I know what I'm doing - I don't expect my edit to be undone. It's too much magic.

This is fixed by this PR.

let's say we have a work branch "foo1.2": all PRs would be renamed as "[1.2] old pr title" - again, not what we expect.

This won't happen because this workflow is run only if the target branch matches "release_**" .

@jdavcs
Copy link
Member

jdavcs commented Nov 22, 2024

This would obviously be nice to have, but was never part of the initial implementation, so it can't be held against this bugfix.

The initial implementation introduced the change that makes these problematic cases possible. I think undoing that change is the obvious way to prevent these cases from happening. That said, if you think the benefits of automating the title format outweigh the potential problems that may cause, I won't object.

@arash77
Copy link
Collaborator

arash77 commented Nov 22, 2024

I think for now the best thing to do is that on editing if it is necessary the bot just warn by commenting on the PR.

@arash77
Copy link
Collaborator

arash77 commented Nov 22, 2024

This is also a problem when we change the base branch from one release to another release, The version will be duplicated.

Also:
- Update title of reopened PRs too.
- Don't unnecessarily checkout the repo.
@nsoranzo nsoranzo force-pushed the release_24.0_check_base_ref_changed branch from 80e76da to 09d8695 Compare November 22, 2024 12:17
@nsoranzo
Copy link
Member Author

The initial implementation introduced the change that makes these problematic cases possible. I think undoing that change is the obvious way to prevent these cases from happening. That said, if you think the benefits of automating the title format outweigh the potential problems that may cause, I won't object.

That's indeed exactly my point, the title update on edited helps with a real problem: reviewers may not notice the change in the target branch, which is important because we have different criteria for PRs targeting a release branch.
If there is a duplication of prefixes in the PR title, it's just mostly an aesthetic issue that's easily fixable manually. If it actually becomes a recurring issue, I'm sure we can fix it.

I think for now the best thing to do is that on editing if it is necessary the bot just warn by commenting on the PR.

https://xkcd.com/1205/ 😉

This is also a problem when we change the base branch from one release to another release, The version will be duplicated.

@arash77 I've discussed it above, but if you'd like to work on that, I'd be happy to review it.

@jdavcs jdavcs changed the base branch from release_24.0 to release_24.2 November 22, 2024 15:16
@jdavcs jdavcs changed the title [24.0] Update title of edited PRs only if base ref changed [24.2] Update title of edited PRs only if base ref changed Nov 22, 2024
@github-actions github-actions bot changed the title [24.2] Update title of edited PRs only if base ref changed [24.2] [24.0] Update title of edited PRs only if base ref changed Nov 22, 2024
@jdavcs
Copy link
Member

jdavcs commented Nov 22, 2024

Changing base to 24.2, right? (it updates #18835 which is 24.2)

@jdavcs jdavcs changed the title [24.2] [24.0] Update title of edited PRs only if base ref changed [24.2] Update title of edited PRs only if base ref changed Nov 22, 2024
@arash77
Copy link
Collaborator

arash77 commented Nov 22, 2024

Changing base to 24.2, right? (it updates #18835 which is 24.2)

I think it was 24.0 #19019

@jdavcs jdavcs changed the base branch from release_24.2 to release_24.0 November 22, 2024 15:28
@github-actions github-actions bot changed the title [24.2] Update title of edited PRs only if base ref changed [24.0] [24.2] Update title of edited PRs only if base ref changed Nov 22, 2024
@jdavcs jdavcs changed the title [24.0] [24.2] Update title of edited PRs only if base ref changed [24.0] Update title of edited PRs only if base ref changed Nov 22, 2024
@jdavcs jdavcs merged commit d59f7a6 into galaxyproject:release_24.0 Nov 22, 2024
51 of 57 checks passed
@jdavcs jdavcs mentioned this pull request Nov 22, 2024
4 tasks
@nsoranzo nsoranzo deleted the release_24.0_check_base_ref_changed branch November 22, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants